Fix Dimensions not updating on Android by jonnyandrew · Pull Request #31973 · facebook/react-native

jonnyandrew · 2021-08-06T09:01:15Z

Summary

When retrieving the device dimensions through the JS Dimensions utility, the result of Dimensions.get can be incorrect on Android.

Related issues

The issue is caused by the Android DeviceInfoModule that provides initial screen dimensions and then subsequently updates those by emitting didUpdateDimensions events. The assumption in that implementation is that the initial display metrics will not have changed prior to the first check for updated metrics. However that is not the case as the device may be rotated (as shown in the attached video).

The solution in this PR is to keep track of the initial dimensions for comparison at the first check for updated metrics.

Changelog

[Android] [Fixed] - Fix Dimensions not updating

Test Plan

Steps to reproduce

Install the RNTester app on Android from the main branch.
Set the device auto-rotation to ON
Start the RNTester app
While the app is loading, rotate the device
Navigate to the Dimensions screen
Either
a. Observe the screen width and height are reversed, or
b. Quit the app and return to step 3.

Verifying the fix

Manually

Using the above steps, the issue should no longer be reproducible.

Automatically

See unit tests in ReactAndroid/src/test/java/com/facebook/react/modules/deviceinfo/DeviceInfoModuleTest.java

Video

Android-dimensions-issue.mp4

facebook-github-bot · 2021-08-06T09:01:20Z

Hi @jonnyandrew!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

analysis-bot

Code analysis results:

google-java-format found some issues. See https://github.com/google/google-java-format

analysis-bot · 2021-08-06T09:31:48Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: fcead14

analysis-bot · 2021-08-06T09:35:41Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	9,188,736	-364
android	hermes	armeabi-v7a	8,714,359	-378
android	hermes	x86	9,630,600	-537
android	hermes	x86_64	9,598,343	-391
android	jsc	arm64-v8a	10,825,093	-285
android	jsc	armeabi-v7a	9,741,997	-281
android	jsc	x86	10,862,373	-446
android	jsc	x86_64	11,471,617	-295

Base commit: fcead14

facebook-github-bot · 2021-08-06T09:47:30Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

analysis-bot

Code analysis results:

google-java-format found some issues. See https://github.com/google/google-java-format

analysis-bot

Code analysis results:

google-java-format found some issues. See https://github.com/google/google-java-format

lunaleaps

Interesting, it seems we have a lot of unused code in DisplayMetrics. I'm wondering if you've tested as well with split screen and verified there are no changes with that behavior.

lunaleaps · 2021-08-11T19:26:11Z

      // Don't emit an event to JS if the dimensions haven't changed
-      WritableNativeMap displayMetrics =
-          DisplayMetricsHolder.getDisplayMetricsNativeMap(mFontScale);
+      WritableMap displayMetrics = DisplayMetricsHolder.getDisplayMetricsWritableMap(mFontScale);


What was the logic behind WritableNativeMap and WritableMap? Sorry I'm not familiar with the difference

WritableNativeMap is an implementation of the WritableMap interface (and that is the implementation that is returned from DisplayMetricsHolder). But in Java-only test code, WritableNativeMap is not supported and needs to be switched out for JavaOnlyMap. So in order to test this class, it needs to depend on WritableMap instead.

jonnyandrew · 2021-08-12T10:56:52Z

Thanks for the review @lunaleaps! I have not tested in split screen but I will take a look. And if you have any particular split screen test cases in mind, please let me know.

jonnyandrew · 2021-08-12T12:51:48Z

@lunaleaps I have run a few manual tests in split screen mode as requested, and I did not see any change in behaviour (except this fix as expected).

Test case	Before	After
1. Given app is not in split screen, when entering split screen, then dimensions are not changed	Pass ✅	Pass ✅
2. Given app is in split screen, when rotated, then dimensions are reversed	Pass ✅	Pass ✅
3. Given app is initialising in split screen, when rotated, then final dimensions are correct	Flaky ❌	Pass ✅

I ran the tests on a Pixel 3a emulator with Android 10. Let me know if you'd like me to post the video recordings.

facebook-github-bot · 2021-08-14T00:20:19Z

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-08-18T01:10:40Z

@lunaleaps merged this pull request in c18a492.

Summary: When retrieving the device dimensions through the JS `Dimensions` utility, the result of `Dimensions.get` can be incorrect on Android. - facebook#29105 - facebook#29451 - facebook#29323 The issue is caused by the Android `DeviceInfoModule` that provides initial screen dimensions and then subsequently updates those by emitting `didUpdateDimensions` events. The assumption in that implementation is that the initial display metrics will not have changed prior to the first check for updated metrics. However that is not the case as the device may be rotated (as shown in the attached video). The solution in this PR is to keep track of the initial dimensions for comparison at the first check for updated metrics. [Android] [Fixed] - Fix Dimensions not updating Pull Request resolved: facebook#31973 Test Plan: 1. Install the RNTester app on Android from the `main` branch. 2. Set the device auto-rotation to ON 3. Start the RNTester app 4. While the app is loading, rotate the device 5. Navigate to the `Dimensions` screen 6. Either a. Observe the screen width and height are reversed, or b. Quit the app and return to step 3. Using the above steps, the issue should no longer be reproducible. See unit tests in `ReactAndroid/src/test/java/com/facebook/react/modules/deviceinfo/DeviceInfoModuleTest.java` https://user-images.githubusercontent.com/4940864/128485453-2ae04724-4ac5-4267-a59a-140cc3af626b.mp4 Reviewed By: JoshuaGross Differential Revision: D30319919 Pulled By: lunaleaps fbshipit-source-id: 52a2faeafc522b1c2a196ca40357027eafa1a84b

Fix Dimensions not updating on Android

ebf8280

analysis-bot suggested changes Aug 6, 2021

View reviewed changes

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 6, 2021

analysis-bot suggested changes Aug 6, 2021

View reviewed changes

jonnyandrew force-pushed the fix/dimensions branch from 333b832 to d638ea4 Compare August 6, 2021 11:46

analysis-bot suggested changes Aug 6, 2021

View reviewed changes

Fix code style issues

3a87a4b

jonnyandrew force-pushed the fix/dimensions branch from d638ea4 to 3a87a4b Compare August 6, 2021 13:42

elicwhite added the Needs: React Native Team Attention label Aug 10, 2021

lunaleaps self-assigned this Aug 11, 2021

lunaleaps requested changes Aug 11, 2021

View reviewed changes

jonnyandrew requested a review from lunaleaps August 12, 2021 13:17

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 12, 2021

facebook-github-bot closed this in c18a492 Aug 18, 2021

facebook-github-bot added the Merged This PR has been merged. label Aug 18, 2021

Conversation

jonnyandrew commented Aug 6, 2021

Summary

Related issues

Changelog

Test Plan

Steps to reproduce

Verifying the fix

Manually

Automatically

Video

Uh oh!

facebook-github-bot commented Aug 6, 2021

Action Required

Process

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

analysis-bot commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 6, 2021

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

lunaleaps left a comment

Choose a reason for hiding this comment

Uh oh!

lunaleaps Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

jonnyandrew Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonnyandrew commented Aug 12, 2021

Uh oh!

jonnyandrew commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 14, 2021

Uh oh!

facebook-github-bot commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

analysis-bot commented Aug 6, 2021 •

edited

Loading

analysis-bot commented Aug 6, 2021 •

edited

Loading

jonnyandrew Aug 12, 2021 •

edited

Loading

jonnyandrew commented Aug 12, 2021 •

edited

Loading